feat(bookmarks): add message bookmarks (MSC4438)#601
Conversation
396ee68 to
2a9fa52
Compare
There was a problem hiding this comment.
Pull request overview
Adds MSC4438 “message bookmarks” to the client by persisting a per-user bookmark index + items in Matrix account data, wiring startup sync + UI entry points, and gating rollout behind both an operator experiment flag and a per-user experimental toggle.
Changes:
- Introduces bookmark domain/repository layers (account-data types, CRUD, validation, orphan/tombstone handling) plus Jotai state + init hook to keep local cache in sync.
- Adds Bookmarks UI (sidebar entries, routes, list/filter/group viewer, remove/restore flows) and message context-menu integration.
- Adds experiment framework support in client config, settings toggle UI, and unit tests for domain/repository/state.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/matrix/accountData.ts | Adds MSC4438 account-data event types/prefixes. |
| src/app/state/settings.ts | Adds per-user experimental toggle (enableMessageBookmarks). |
| src/app/state/bookmarks.ts | Adds bookmark Jotai atoms + derived ID Set. |
| src/app/state/bookmarks.test.ts | Tests derived bookmark ID Set atom behavior. |
| src/app/pages/Router.tsx | Adds Bookmarks route under Home and Inbox. |
| src/app/pages/pathUtils.ts | Adds helpers for Home/Inbox bookmarks paths. |
| src/app/pages/paths.ts | Adds bookmarks/ path segment + concrete paths. |
| src/app/pages/client/inbox/Inbox.tsx | Adds Inbox sidebar nav item gated by experiment/setting. |
| src/app/pages/client/home/Home.tsx | Adds Home sidebar nav item gated by experiment/setting. |
| src/app/pages/client/ClientNonUIFeatures.tsx | Mounts bookmark init hook; adjusts in-app audio gating. |
| src/app/pages/client/bookmarks/index.ts | Re-exports Bookmarks page module. |
| src/app/pages/client/bookmarks/BookmarksList.tsx | Implements Bookmarks list UI (filter/group/jump/remove/restore). |
| src/app/hooks/useClientConfig.ts | Adds experiments config typing + deterministic assignment helpers/hook. |
| src/app/hooks/router/useInbox.ts | Adds selection matcher for Inbox Bookmarks route. |
| src/app/hooks/router/useHomeSelected.ts | Adds selection matcher for Home Bookmarks route. |
| src/app/features/settings/experimental/MSC4438MessageBookmarks.tsx | Adds experimental settings tile for bookmarks toggle. |
| src/app/features/settings/experimental/Experimental.tsx | Wires bookmarks toggle into Experimental settings section. |
| src/app/features/room/message/Message.tsx | Adds message context-menu item to add/remove bookmark. |
| src/app/features/bookmarks/useInitBookmarks.ts | Initializes and keeps bookmark atoms synced with account data. |
| src/app/features/bookmarks/useBookmarks.ts | Adds read + action hooks for bookmarks. |
| src/app/features/bookmarks/bookmarkRepository.ts | Implements MSC4438 account-data CRUD + listing/orphan recovery. |
| src/app/features/bookmarks/bookmarkRepository.test.ts | Adds repository unit tests. |
| src/app/features/bookmarks/bookmarkDomain.ts | Defines bookmark types, ID algorithm, validators, item builder. |
| src/app/features/bookmarks/bookmarkDomain.test.ts | Adds domain unit tests + reference vectors. |
| config.json | Adds operator-controlled experiments.messageBookmarks flag. |
| .changeset/message-bookmarks.md | Adds release note entry for message bookmarks feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
the amount of commits and line changes this pr wants to merge seems a bit off for the problem stated 🤔 |
d82f8e4 to
879cc78
Compare
7464afe to
8cc09fa
Compare
7c5ead8 to
9a0c01f
Compare
… not in local timeline
…nder picker - Add enableBookmarkReminders setting (default false, requires enableMessageBookmarks to be shown in settings) - Show a 'Bookmark Reminders' sub-toggle in the MSC4438 experimental settings tile, visible only when bookmarks are enabled - Gate RemindersFeature on both enableMessageBookmarks and enableBookmarkReminders so the SW only receives reminder updates when the feature is active - New reminderRepository: setBookmarkReminder / clearBookmarkReminder / listReminders — read/write the moe.sable.bookmarks.reminders account data event - New useBookmarkReminders hook: reads reminders from account data and stays live via useAccountDataCallback - New useBookmarkReminderActions hook: set and clear reminder callbacks - Add Bell/BellRing icon button to each active bookmark in BookmarksList (visible when reminders setting is on); clicking opens an inline datetime-local picker with Set/Clear actions
BookmarksTab was reusing InboxTab's navToActivePath logic, so if the user had previously visited inbox/notifications, clicking the bookmarks button would navigate there instead of inbox/bookmarks. The tab now always navigates directly to getInboxBookmarksPath(), which also fixes the mobile regression where no navigation occurred (InboxTab's mobile guard was absent here).
After checkDueReminders fires a bookmark reminder notification, the SW
persists only the remaining (non-due) reminders to its cache — but
useReminderSync would immediately overwrite that cache by pushing the
full list from Matrix account data (which hadn't been updated).
Fix: the SW now posts a { type: 'remindersFired', bookmarkIds } message
to all open window clients after firing. useReminderSync listens for
this message and calls clearBookmarkReminder for each fired ID, updating
account data so the next syncReminders call pushes the correct subset.
…nimal purge tombstone
- sw.ts: include isReminder in the notificationClick postMessage so the
running app can handle reminder taps correctly without falling through
to the "no roomId" early return.
- ClientNonUIFeatures.tsx: HandleNotificationClick now opens the
bookmarks panel (bookmarksPanelAtom) when isReminder is true, instead
of doing nothing (previously bailed out at the !roomId guard).
- bookmarkRepository.ts: purgeBookmark now writes the minimal tombstone
{ deleted: true, purged: true } instead of spreading the original
bookmark content. The original room/event metadata no longer lingers
in account data after a permanent archive deletion.
The bell icon + datetime picker were only wired up in the panel/inbox variant (features/bookmarks/BookmarksList). The full-page view rendered via the router (pages/client/bookmarks/BookmarksList) had no reminder UI at all. Add useBookmarkReminders + useBookmarkReminderActions to BookmarkItemRow so the bell icon and inline picker appear there too when enableBookmarkReminders is enabled.
BookmarksPanel and its associated BookmarksList in features/bookmarks were never wired into the router or any live component. The full-page bookmarks view at pages/client/bookmarks/BookmarksList is the only real implementation. Also fix HandleNotificationClick: bookmarksPanelAtom never existed, so reminder notification taps were silently broken. Navigate to the inbox bookmarks route instead.
9a0c01f to
bca977e
Compare
…debar tab - Add useFiredReminderCount() hook to track reminders that have fired but not been cleared - Display badge with count on bookmarks tab when reminders are pending - Badge updates every minute to catch newly-fired reminders - Icon filled state remains tied to whether bookmarks page is currently open
- BookmarksTab: show filled bookmark icon when bookmarks exist (not just when the panel is selected) — glanceable indicator restored - useClientConfig: re-export EXPERIMENT_OVERRIDE_PREFIX so consumers can import it instead of duplicating the string literal - ExperimentsPanel: import EXPERIMENT_OVERRIDE_PREFIX from useClientConfig instead of re-declaring it locally
…-app reminder routing - ClientNonUIFeatures: add ReminderBanners component that listens for remindersInApp SW messages and shows an in-app banner; wire into RemindersFeature alongside ReminderSync - sw.ts: persist appVisibleAt timestamp so cold SW restarts on iOS/iPad can still suppress duplicate OS notifications within 30 s of the app being visible; call persistSettings() on setAppVisible so the timestamp is written immediately - sw.ts: in checkDueReminders(), route due reminders as in-app banners (remindersInApp postMessage) when a visible tab is open, falling back to OS showNotification() otherwise — avoids duplicate alerts on iPad
Two SW fixes for bookmark reminders: 1. Cold-start notification click: the previous code built a /to/:roomId/:eventId URL which ToRoomEvent misparses (it expects /to/:userId/:roomId/:eventId). Navigate to /inbox/bookmarks/ instead — consistent with the warm-start path where HandleNotificationClick already calls navigate(getInboxBookmarksPath()). 2. checkDueReminders: switch from Promise.all to Promise.allSettled so that persistReminders() and the remindersFired postMessage always execute, even when showNotification() rejects (e.g. notification permission has been revoked). Without this, fired reminders stayed in the SW cache and would re-fire on every subsequent 60 s interval.
datetime-local inputs expect a local-time value (YYYY-MM-DDTHH:MM). Using toISOString() produced a UTC timestamp instead, so users in negative UTC offsets (e.g. EDT UTC-4) would see the stored UTC time (00:15) rather than the local time they entered (20:15).
- BookmarksTab: show filled bookmark icon when bookmarks exist (not just when the panel is selected) — glanceable indicator restored - useClientConfig: re-export EXPERIMENT_OVERRIDE_PREFIX so consumers can import it instead of duplicating the string literal - ExperimentsPanel: import EXPERIMENT_OVERRIDE_PREFIX from useClientConfig instead of re-declaring it locally
Two bugs fixed: 1. Reminders only fired on one device The SW sent a 'remindersFired' message when it showed an in-app banner, which caused the receiving device (whichever had the app open) to delete the reminder from Matrix account data. That prevented other devices from ever firing their own copy. Fix: remove the remindersFired auto-clear entirely. Each device now tracks a firedAt timestamp in its own SW cache (sable-reminders-v1). checkDue- Reminders skips entries that already have firedAt set, and updateReminders preserves firedAt when the same bookmarkId/remindAt pair is re-synced. The user dismisses the reminder explicitly via 'Clear Reminder'. 2. 'Clear reminder' / overdue chip didn't update reliably clearBookmarkReminder wrote to the server via setAccountData. The local UI only updated when the sync loop echoed the change back, which could take seconds — making it look like the clear had failed. Additionally, the concurrent remindersFired auto-clear + manual clear created a read-modify-write race (both read the current list, filter, write — the second write would win with stale data). Fix: introduce remindersAtom (global Jotai state). useBookmarkReminderActions now performs an optimistic atom update before the server write, so the UI responds instantly. useReminderSync is the single source of truth when real account data arrives.
…types locally These types were only defined in feat/developer-tools. Since message-bookmarks uses selectExperimentVariant / useExperimentVariant independently, define the types directly in useClientConfig.ts and add experiments to ClientConfig.
|
Thanks for the note! The branch was initially developed on top of my integration branch (which merges all feature branches together for local testing), which bloated the commit/diff count significantly beyond what the feature itself required. The branch has since been rebased onto |
Description
Implements MSC4438 message bookmarks — lets users save any room event to a personal bookmark list stored in Matrix account data.
Commits in this PR:
feat(bookmarks): add message bookmarks (MSC4438)— full feature implementation:BookmarkItemContent/BookmarkIndextypes,bookmarkRepository.tsCRUD operations (addBookmark,removeBookmark,listBookmarks,useInitBookmarks), auseBookmarksReact hook, context menu integration, and the bookmarks viewer panel.test(bookmarks): add unit tests for MSC4438 bookmark domain and repository— covers the core repository logic and hook behaviour.fix(bookmarks): add missing focusId to MSC4438 settings SettingTile— prevents a React key-prop warning that appeared in the Settings modal.fix(bookmarks): soft-delete item before updating index in removeBookmark— corrects the write order inremoveBookmarkto mirror the item-first ordering ofaddBookmark, preventing orphan-recovery inlistBookmarksfrom transiently resurrecring a bookmark between the two writes.fix(bookmarks): wire useInitBookmarks and fix orphan tombstoning— mountsBookmarksFeatureinClientNonUIFeaturessouseInitBookmarks()is actually called on startup (the bookmark list was never populated without this). Also replacesreadItem()(which validates and may return null for malformed items) with a direct account-data read in the tombstone path, so malformed or partially-written bookmark items can always be fully deleted. Includes regression tests for both cases.Fixes #
Type of change
Checklist:
AI disclosure:
The
bookmarkRepository.tsCRUD helpers and theuseInitBookmarkshook skeleton were drafted with AI assistance and reviewed against the MSC4438 spec and Sable's existing account-data patterns. The tombstone fix logic and theClientNonUIFeatureswiring were written manually after tracing the runtime call graph.